Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASoC: SOF: pcm: Improve debug and error prints #5252

Open
wants to merge 3 commits into
base: topic/sof-dev
Choose a base branch
from

Conversation

ujfalusi
Copy link
Collaborator

Hi,

Our error printing alone leaves out information which can provide a needed context for the error itself.
For example in IPC3: "ipc tx error for 0x60010000 (msg/reply size: 108/20): -22"
This will not tell which PCM device, what direction this error had happened.

Similarly the debug prints are using ad-hoc formatting and they are not consistent, one needs to look for prior or later prints for context, which can be problematic with concurrent PCM operation.

At the heart this series will switch to a spcm_dbg() and spcm_err() wrapper to include useful information in prints where spcm is available.

As preparation, the PCM functions are relocated to pcm.c from sof-audio.c.

lgirdwood
lgirdwood previously approved these changes Nov 26, 2024
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one @ujfalusi - do you think similar can be done for kcontrols

@ujfalusi
Copy link
Collaborator Author

Nice one @ujfalusi - do you think similar can be done for kcontrols

I'm going through the files and logs to see what can be improved. With a good log we can cut down on some back and forth to get the logs which have information and in case if it is a hard to reproduce issue, that can take a while...

* The spcm_dbg() is prepared to handle a call without valid direction (dir == -1)
*/
#define spcm_dbg(__spcm, __dir, __fmt, ...) { \
if (__dir == -1) \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ujfalusi this is really used only at the time of creation. Do we really need to special handle it just for that? In all practical cases, I think we're going to have the dir defined.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is only in one case, I can put it as __unlikely(), printing the dir and a number would be confusing to look at if the direction is not valid?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no what i was suggesting was that we leave out the new macro for this one case and use it everywhere else with a valid direction.

Move the sof_pcm_stream_free() from sof-audio.c to pcm.c as static function
and add wrapper to free all active stream, which is going to be used in
ipc3/4 topology code (removes duplicated code).

With this change most of the PCM stream related code is located in one
source file for easier lookup and simplified flow.

Signed-off-by: Peter Ujfalusi <[email protected]>
… open

The platform specific pcm_open call via snd_sof_pcm_platform_open() can
modify the initial buffer configuration via constraints.

Move the prints as last step in the sof_pcm_open() function to reflect the
final setup.

Signed-off-by: Peter Ujfalusi <[email protected]>
…ev_err()

Introduce spcm_dbg() and spcm_err() macros to provide consistent printing
for debug and error messages which includes usable information in the
print's prefix.

Update the prints in pcm.c, ipc3-pcm.c and ipc4-pcm.c to take advantage of
the features provided by the macros.

Signed-off-by: Peter Ujfalusi <[email protected]>
@ujfalusi
Copy link
Collaborator Author

ujfalusi commented Dec 2, 2024

Changes since v1:

  • use dev_dbg() directly at entry of sof_pcm_new and drop the __dir = -1 handling from the spcm_dbg() macro.

@ranj063
Copy link
Collaborator

ranj063 commented Dec 3, 2024

SOFCI TEST

1 similar comment
@ranj063
Copy link
Collaborator

ranj063 commented Dec 5, 2024

SOFCI TEST

@lgirdwood
Copy link
Member

SOFCI TEST

The current results look as expected, I will rerun again though as some DUTs just got a BIOS update.

@lgirdwood
Copy link
Member

SOFCI TEST

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants